feat(event): emit typed error envelopes across the event domain#1289
feat(event): emit typed error envelopes across the event domain#1289evandance wants to merge 1 commit into
Conversation
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors shortcuts/event and cmd/event to use project-typed ChangesEvent error handling standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@770e93fb54624d03690090ab7c6c1aee2e867661🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-event -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1289 +/- ##
==========================================
+ Coverage 71.01% 71.22% +0.21%
==========================================
Files 681 682 +1
Lines 65435 65489 +54
==========================================
+ Hits 46470 46646 +176
+ Misses 15320 15191 -129
- Partials 3645 3652 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e453130 to
2182b96
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/event/subscribe.go (1)
145-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
--output-dir,--filter, and--routevalidation intoValidate.These checks only run inside
Execute, soevent +subscribe --dry-runcan still succeed for inputs that a real run will reject. That makes dry-run lie about request validity for the flags this PR is migrating. Please extract a shared validator and invoke it fromValidateso dry-run and real execution fail the same way.As per coding guidelines, "Dry-run E2E tests required for every shortcut change must validate request structure without calling real APIs ... using
--dry-runflag." Based on learnings, the shortcutValidatecallback runs before dry-run handling, so checks left inExecutenever run under--dry-run.Also applies to: 185-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/event/subscribe.go` around lines 145 - 152, Extract the output-dir, filter, and route flag checks out of Execute and into the shortcut's Validate method by creating a shared validator function (e.g., validateSubscribeParams or similar) that is called from Validate and reused by Execute; move the validate.SafeOutputPath call and its error handling (currently returning eventValidationParamErrorWithCause for "--output-dir") into that shared validator, add equivalent validation for the --filter and --route flags there, and then call this validator from Execute to avoid duplication so both Validate and Execute use the same logic.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@shortcuts/event/subscribe.go`:
- Around line 145-152: Extract the output-dir, filter, and route flag checks out
of Execute and into the shortcut's Validate method by creating a shared
validator function (e.g., validateSubscribeParams or similar) that is called
from Validate and reused by Execute; move the validate.SafeOutputPath call and
its error handling (currently returning eventValidationParamErrorWithCause for
"--output-dir") into that shared validator, add equivalent validation for the
--filter and --route flags there, and then call this validator from Execute to
avoid duplication so both Validate and Execute use the same logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55228a9a-f49a-41a3-81c4-ab93a4547768
📒 Files selected for processing (11)
.golangci.ymllint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/event/errors.goshortcuts/event/pipeline.goshortcuts/event/processor_test.goshortcuts/event/registry.goshortcuts/event/router.goshortcuts/event/subscribe.gotests/cli_e2e/event/event_subscribe_dryrun_test.gotests/cli_e2e/event/event_subscribe_error_test.go
✅ Files skipped from review due to trivial changes (1)
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/cli_e2e/event/event_subscribe_dryrun_test.go
- shortcuts/event/registry.go
- shortcuts/event/errors.go
- shortcuts/event/router.go
- shortcuts/event/processor_test.go
2182b96 to
f7006a6
Compare
f7006a6 to
d08f21c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/event/runtime_test.go (1)
37-52: ⚡ Quick winConsider the established TestFactory+override pattern for HTTP stubbing.
Per coding guidelines and the learning from PR
#1151, the accepted pattern for tests with custom HTTP response control is:
- Call
cmdutil.TestFactory(t, nil)for canonical Factory wiring- Set
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())for config isolation- Override
f.HttpClientwith a customhttp.RoundTripperThis test manually constructs the
consumeRuntimeand its dependencies. While this works for pure unit tests, usingTestFactoryensures consistency and catches any future Factory-wiring changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/event/runtime_test.go` around lines 37 - 52, The test helper newTestConsumeRuntime currently constructs a consumeRuntime and its client manually; change the test to use the canonical TestFactory wiring: call cmdutil.TestFactory(t, nil) to get the factory, call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate config, then override f.HttpClient (the factory's HTTP client) by setting f.HttpClient.Transport to your custom http.RoundTripper before creating or injecting the consumeRuntime so the test uses the factory-provided client wiring instead of newTestConsumeRuntime's manual construction.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/event/runtime_test.go`:
- Around line 37-52: The test helper newTestConsumeRuntime currently constructs
a consumeRuntime and its client manually; change the test to use the canonical
TestFactory wiring: call cmdutil.TestFactory(t, nil) to get the factory, call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate config, then
override f.HttpClient (the factory's HTTP client) by setting
f.HttpClient.Transport to your custom http.RoundTripper before creating or
injecting the consumeRuntime so the test uses the factory-provided client wiring
instead of newTestConsumeRuntime's manual construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c57a4985-a126-4b11-9c08-d7bad24aa647
📒 Files selected for processing (19)
.golangci.ymlcmd/event/consume.gocmd/event/consume_test.gocmd/event/preflight_test.gocmd/event/runtime.gocmd/event/runtime_test.gocmd/event/schema.gocmd/event/schema_test.gocmd/event/suggestions.golint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/event/errors.goshortcuts/event/pipeline.goshortcuts/event/processor_test.goshortcuts/event/registry.goshortcuts/event/router.goshortcuts/event/subscribe.gotests/cli_e2e/event/event_subscribe_dryrun_test.gotests/cli_e2e/event/event_subscribe_error_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- lint/errscontract/rule_no_legacy_envelope_literal.go
- lint/errscontract/rule_no_legacy_common_helper_call.go
- tests/cli_e2e/event/event_subscribe_dryrun_test.go
- cmd/event/preflight_test.go
- shortcuts/event/registry.go
- shortcuts/event/pipeline.go
- shortcuts/event/router.go
- shortcuts/event/errors.go
- cmd/event/runtime.go
- cmd/event/schema.go
- .golangci.yml
- shortcuts/event/subscribe.go
- cmd/event/consume.go
3d0798e to
0e3746e
Compare
Replace every command-facing error path in the event domain — the consume/schema command layer, the +subscribe shortcut, EventKey definitions, and the consume orchestration — with typed errs.* envelopes, so consumers get stable type, subtype, param, hint, and missing_scopes metadata for classification and recovery instead of free-form message text. - Input validation (--jq, --param, --output-dir, --filter, --route, unknown EventKey, EventKey params) reports validation / invalid_argument with the offending flag in param and an actionable hint. - Scope preflight reports authorization / missing_scope with the machine-readable missing_scopes list; console-subscription and single-bus preconditions report failed_precondition with recovery hints. - The consume API boundary passes already-typed errors through and classifies transport, non-JSON HTTP, and unparsable responses; the vc note-detail retry now matches the not-found code on typed errors (it silently never fired against the legacy envelope shape). - Previously-bare failures exited 1 with a plain-text "Error:" line and now exit with their category code (validation 2, auth 3, network 4, internal 5) alongside the typed stderr envelope. - forbidigo and errscontract guards now cover the event paths so regressions fail lint; AGENTS.md and the lark-event skill document the typed contract for agent consumers. Validation: make unit-test (race) green; event unit and e2e suites assert category/subtype/param/hint and cause preservation against the real binary; errscontract and golangci lint clean.
0e3746e to
770e93f
Compare
Summary
The event domain still returned command-facing failures through legacy
output.Err*helpers and final barefmt.Errorfpaths, so callers had to recover validation, auth, local file, lock, and WebSocket failure semantics from prose. This migrates every event error producer — theevent consume/event schemacommand layer, theevent +subscribeshortcut, the EventKey definitions underevents/, and the consume orchestration ininternal/event/(event list/status/stopalready produced no envelope errors of their own) — to typederrs.*envelopes, giving scripts and AI agents stabletype,subtype,param,missing_scopes, and cause-preserving errors.This PR is intentionally scoped to the event domain's own command-returned error producers. The event domain has no batch partial-failure flows, so there is no
OutPartialFailuremigration in this change. Failures inside the long-running WebSocket callback (malformed event JSON, per-event file write errors) remain non-fatal stderr diagnostics: a single bad event must not terminate the subscriber.event status --fail-on-orphankeeps its bare exit-code signal in both output modes;event stopkeeps it only in text mode — with--jsonthe per-target outcomes are already on stdout and the command exits 0. Bus-daemon-internal errors (protocol frames, probe/handshake intermediates, the remote-connection check that degrades to a stderr warning) never reach a command envelope and stay as wrapped intermediates; every command exit lifts them into a typed error with the cause preserved.Changes
Command layer (
cmd/event/):event consumepreflight failures to typed errors:authorization/missing_scopewithmissing_scopes,identity, and an identity-appropriate grant hint (wiretypechanges fromauthtoauthorization; exit code stays 3)validation/failed_preconditionwith a console subscription hintevent consumeinput validation tovalidation/invalid_argumentwithparam:--jq,--param, and--output-dir; sentinel errors stayerrors.Is-comparable as typed causes.authentication/token_missing, passing through already-typed credential errors unchanged.consumeRuntime.CallAPI): transport failures classify asnetwork/transport, non-JSON HTTP errors and unparsable response bodies asinternal/invalid_response; already-typed errors pass through unchanged. OAPI envelope failures were already typed viaerrclass.BuildAPIError.Consume orchestration (
internal/event/consume/) and EventKey definitions (events/):consume.Runfatal exits to typed errors: unknown EventKey and--paramvalidation ->validation/invalid_argumentwithparamand anevent schemahint; jq compile failures ->validation/invalid_argumentwithparam: --jq; bus handshake / pre-consume / daemon startup failures -> typed internal errors with causes and recovery hints; output-dir creation ->internal/file_io.validation/failed_preconditionwith anevent status/event stophint.events/vc,events/minutes,events/whiteboard): missingwhiteboard_id->validation/invalid_argumentwithparam: --paramand a hint; missing runtime client invariants -> typed internal errors.events/vcnote-detail retry: it matched the not-found API code through the legacy*output.ExitErrorshape, which typed API errors no longer carry, so the retry never fired; it now readserrs.ProblemOf(err).Code.lint/errscontract's legacy-runtime-API rule to files importingshortcuts/common, so same-named typed methods (the event domain'sAPIClient.CallAPI) do not false-positive.Error:line and now exit with their category code (validation 2, authentication/authorization 3, network 4, internal 5) alongside the typed stderr envelope.event _busdaemon command's logger-setup and run-loop exits into typed internal errors (visible inbus.logonly).skills/lark-eventexit-code table: startup/runtime failures are documented as typed JSON envelopes with per-category exit codes, steering agents to parseerror.type/error.subtype/error.param/error.hintinstead of message text.Shortcut layer (
shortcuts/event/):shortcuts/event/errors.gowith event-local typed helpers for validation, file I/O, and WebSocket transport failures. Cause-wrapping helpers take the cause as their first argument, append it to the message, and preserve it forerrors.Unwrap.event +subscribesetup failures from legacyoutput.ErrValidation/output.ErrNetwork/ final bare wrapping to typederrs.*builders:--output-dir->validation/invalid_argumentwithparam: --output-dir--filter->validation/invalid_argumentwithparam: --filtervalidation/failed_preconditionwith a recovery hint;paramstays empty because the user supplied no offending flaginternal/file_ionetwork/transportParseRoutesto typed validation errors withparam: --route, preserving regex/path-validation causes.internal/file_io.fmt.Errorfto a typed internal error.Guards and tests:
AGENTS.mderror-handling convention from the legacyoutput.Err*helpers to the typederrs.*contract: a constructor cheat sheet, exact signatures for the three easiest-to-misuse primitives (CallAPITyped,ProblemOf,WithParamscope), the pass-through rule, and theparam/ hint / cause conventions, linked toerrs/ERROR_CONTRACT.md— so new contributions emit typed envelopes from the start.cmd/event/,events/, andshortcuts/event/to the migrated-path guards (errs-typed-only,errs-no-bare-wrap,errs-no-legacy-helper) and the errscontract manifests;internal/event/consume/joinserrs-typed-onlyand the manifests while keeping wrapped intermediates, matching thecmd/auth/cmd/configprecedent.*output.ExitErrorshape assertions to typed assertions, including themissing_scopeswire field; lock the note-detail not-found retry on typed API codes, the single-bus guard'sfailed_precondition, the daemon loggerfile_ioexit, and the pre-consume typed shapes.category/subtype/param) and cause preservation across consume input validation, EventKey param validation, jq compilation, token resolution, the consume API runtime boundary (stubbed transport), subscriber lock contention, router parsing, registry registration, and pipeline directory creation.event +subscribedry-run output shape, and two error-path tests asserting the stderr typed envelope (type/subtype/param/message/hint) for invalid--routeinput and an unknownevent consumeEventKey. None of them opens a WebSocket connection.Test Plan
make buildgo test ./cmd/event ./shortcuts/event ./events/... ./internal/event/...go test ./tests/cli_e2e/event/... -run 'DryRun|Regression'go vet ./...go run -C lint . ..go test -C lint ./...go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main— 0 issuesmake unit-testRelated Issues
N/A